-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(dataset): handle missing python_type gracefully #19553
fix(dataset): handle missing python_type gracefully #19553
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
77e0969
to
b95fb1a
Compare
Codecov Report
@@ Coverage Diff @@
## master #19553 +/- ##
===========================================
- Coverage 66.65% 53.74% -12.92%
===========================================
Files 1680 1680
Lines 64280 64286 +6
Branches 6564 6564
===========================================
- Hits 42849 34550 -8299
- Misses 19729 28034 +8305
Partials 1702 1702
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
* fix(dataset): handle missing python_type gracefully * refactor TEMPORAL_TYPES (cherry picked from commit d9343a4)
* fix(dataset): handle missing python_type gracefully * refactor TEMPORAL_TYPES (cherry picked from commit d9343a4)
🏷️ preset:2022.13 |
* fix(dataset): handle missing python_type gracefully * refactor TEMPORAL_TYPES
SUMMARY
In the new Dataset model, we check if a column is temporal by checking the
python_type
of all columns. However, the property isn't guaranteed to be implemented: https://github.com/sqlalchemy/sqlalchemy/blob/7935b76d9e5b5fd4e64b2c6c3473737186acf2db/lib/sqlalchemy/sql/type_api.py#L618-L635 . Therefore, if calling it raisesNotImplementedError
, we assume the column is not temporal.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION